Deactivate customer #557
Deactivate customer #557
Conversation
Merging main before pushing
There was a problem hiding this comment.
Nice work! I might have challenged some frontend-specific conventions in my comments in the frontend project, please let me know if I have so that I can learn. :)
One last note: I would suggest to generally use maximum one empty line between code lines/blocks.
| @@ -47,7 +49,7 @@ public async Task<ActionResult<List<EngagementPerCustomerReadModel>>> Get( | |||
| var selectedOrgId = await organisationRepository.GetOrganizationByUrlKey(orgUrlKey, ct); | |||
| if (selectedOrgId is null) return BadRequest(); | |||
|
|
|||
| var absenceReadModels = new EngagementPerCustomerReadModel(-1, AbsenceCustomerName, | |||
| var absenceReadModels = new EngagementPerCustomerReadModel(-1, AbsenceCustomerName, true, | |||
There was a problem hiding this comment.
| var absenceReadModels = new EngagementPerCustomerReadModel(-1, AbsenceCustomerName, true, | |
| var absenceReadModels = new EngagementPerCustomerReadModel(-1, AbsenceCustomerName, IsActive: true, |
Consider defining what is true
| @@ -301,7 +318,7 @@ private CustomersWithProjectsReadModel HandleGetAbsenceWithAbsences(string orgUr | |||
| { | |||
| var vacation = new EngagementReadModel(-1, "Ferie", EngagementState.Absence, false); | |||
|
|
|||
| var readModel = new CustomersWithProjectsReadModel(-1, AbsenceCustomerName + " og Ferie", | |||
| var readModel = new CustomersWithProjectsReadModel(-1, AbsenceCustomerName + " og Ferie", true, | |||
There was a problem hiding this comment.
| var readModel = new CustomersWithProjectsReadModel(-1, AbsenceCustomerName + " og Ferie", true, | |
| var readModel = new CustomersWithProjectsReadModel(-1, AbsenceCustomerName + " og Ferie", IsActive: true, |
Consider defining what is true
| table: "Customer", | ||
| type: "bit", | ||
| nullable: false, | ||
| defaultValue: true); |
There was a problem hiding this comment.
How will this default true value be added to existing Customer objects in the database? Does the AddColumn-method call handle this?
There was a problem hiding this comment.
Alternatively: You could replace the property IsActive with property IsDeactivated (the defaultValue would then implicitly be false). Then the existing consumer objects don't need to be handled.
|
|
||
| function toggleListElementVisibility() { | ||
| setIsListElementVisible((old) => !old); | ||
| } | ||
|
|
||
| async function onActivate(customerId: number, active: boolean) { |
There was a problem hiding this comment.
Consider renaming the active variable to a name indicating whether it represents the current state or the target state. active may be interpreted either way.
| async function onActivate(customerId: number, active: boolean) { | ||
| if (isLoading) return; | ||
| setIsLoading(true); | ||
| setIsActive(active); |
There was a problem hiding this comment.
Should this be set here, or after a successful fetch?
| <> | ||
| <div className="sidebar z-10 bg-background_grey h-full flex flex-col gap-6 p-4 w-[300px] overflow-y-auto"> | ||
| <div className="flex flex-row justify-between items-center"> | ||
| <h1 className="">Filter</h1> |
|
|
||
| async function onActivate(customerId: number, active: boolean) { |
There was a problem hiding this comment.
Same comments regarding the active variable here as in CustomerRow.tsx
There was a problem hiding this comment.
Or, perhaps: Could/Should this (de)activation button/logic be a separate component that is used both places?
Added the possibility of deactivating (and activating) customers, with a toggle to filter out the deactivated customers.
